Skip to content

Corrected the sed -i command in script/package shell script [#554]#672

Merged
brntbeer merged 2 commits into
github:masterfrom
bazzaar:bazzaar_script_fix
Apr 4, 2019
Merged

Corrected the sed -i command in script/package shell script [#554]#672
brntbeer merged 2 commits into
github:masterfrom
bazzaar:bazzaar_script_fix

Conversation

@bazzaar
Copy link
Copy Markdown
Contributor

@bazzaar bazzaar commented Mar 31, 2019

Overview

In the training-kit README, a workflow is presented to create a local copy of the files to be served by a web-server. The first step of that workflow is to run script/package, but that step fails ...

This PR fixes two errors in the sed command inside script/package [ see comment by bazzaar in issue #554 ]

I subsequently ran through the corrected workflow on my local version of training-kit, and the web-served files are built and packaged successfully, and then display correctly in Firefox browser using the simple python web server.

Questions

As the sed regex didn't match the text in the _config.yml, perhaps some change had been made to the _config.yml file previously, ... maybe the keywords are no longer as expected in that file? Just a thought ...

hope this helps
bazzaar

@bazzaar
Copy link
Copy Markdown
Contributor Author

bazzaar commented Mar 31, 2019

Whilst I remember, and following on from the above PR, I think the creation of the kit subdirectory, in step 2 of the aforesaid workflow, isn't needed. Therefore in step 3, it's sufficient to un-tar the archive into test_site. Then the step 4 is to cd to test_site, and run the web server from that directory in step 5.

As it is currently documented, I found that, contrary to the instruction in step 5, I had to cd into test_site/kit, in order for the files to be found by the links in the web browser. See what you think, perhaps I'm mistaken.

hope this helps
bazzaar

@brianamarie
Copy link
Copy Markdown
Contributor

@brntbeer I seem to remember you creating the packaging script - would you have time to review this PR? 👀

@brntbeer
Copy link
Copy Markdown
Member

brntbeer commented Apr 2, 2019

@bazzaar first off, welcome! thank you for this contribution.

Sorry for the delay in looking into this, I'll take a look now ! 🎉

Comment thread script/package Outdated
@bazzaar
Copy link
Copy Markdown
Contributor Author

bazzaar commented Apr 2, 2019 via email

Comment thread script/package Outdated
@brntbeer
Copy link
Copy Markdown
Member

brntbeer commented Apr 2, 2019

Did you see my comment in the PR about the possible redundant kit subdir?

Yep, you're right. I'm taking a look at that but would prefer to address that after this PR is merged if you dont mind. that way we can have these two changes be atomic

Co-Authored-By: bazzaar <barryfoxbat@btinternet.com>
@brntbeer
Copy link
Copy Markdown
Member

brntbeer commented Apr 4, 2019

🎉 @bazzaar thank you much for this!

@brntbeer brntbeer merged commit 390c9f8 into github:master Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants